minkipc: Add libraries and applications for security feature support #1094
Conversation
jiaxli-QC
requested review from
lumag,
ndechesne,
ricardosalveti,
sbanerjee-quic and
vkraleti
as code owners
|
Also please drop 'meta-qcom: security: from the commit subjects, use 'Add recipe <>' with short meaning to the recipe. |
|
And split the commits that are including the packages in the packagegroup. |
I don't think we need packagegroup at all. In the end, we only need the |
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
Please go through all review comments and implement all of them or at least provide some response to the comments that think are not correct or not applicable.
| Subject: [PATCH] minkipc:compilation: Fix compilation issue | ||
|
|
||
| - To fix compilation error: #warning "<cstdbool> is deprecated in C++17, remove the #include" [-Werror=cpp]. | ||
| - Disable downloading minkidlc in CMakeLists.txt. |
There was a problem hiding this comment.
And how is it related to disabling downloading of minkidlc?
dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb
Show resolved
Hide resolved
|
Hi @lumag . Currently, minkidlc has only released an x86 version binary for users, and all use cases are based on x86 hosts. More validation is needed on different hosts before expanding minkidlc support. |
OE provides support for building Rust binaries. minkidlc is provided in the source form. It has 0 tests to check for regressions, but that's a different topic. So, no. I don't think we should accept downloading a binary to be executed on the build host. |
jiaxli-QC
force-pushed
the
master
branch
2 times, most recently
from
2f341ae to
4893e2c
Compare
| util-linux-taskset \ | ||
| wpa-supplicant \ | ||
| ${@bb.utils.contains('DISTRO_FEATURES', 'opengl', 'kmscube', '', d)} \ | ||
| packagegroup-qcom-secure \ |
There was a problem hiding this comment.
Now this will break because the package is not existing.
Hi @lumag , Do you mean we should raised the minkidlc compilation change in OE-core repo? I tried to compile the minkidlc in meta-qcom, but encountered some issues. |
Please add the source code for the recipe that you are trying to build. I can't comment the failure without looking at your recipe. |
Here is the bb file that has been created for minkidlc compilation. SUMMARY = "Mink IDL compiler (prebuilt binary)"
DESCRIPTION = " \
Mink IDL is used to describe programming interfaces that can be used to communicate across security domain boundaries. \
Once an interface is described in an IDL source file, the Mink IDL compiler can generate target language header files. \
"
LICENSE = "BSD-3-Clause"
LIC_FILES_CHKSUM = "file://LICENSE.txt;md5=acff25b0ff46523fa016b260dbf64945"
SRC_URI = "git://github.com/quic/mink-idl-compiler.git;branch=main;protocol=https"
SRCREV = "1a52cd017ae21750c23e01851fdffd143f9c85d0"
inherit cargo native
DEPENDS += "rust-native cargo-native"
do_install() {
install -d ${D}${bindir}
install -m 0755 ${S}/target/debug/idlc ${D}${bindir}/minkidlc
}
FILES:${PN} += "${bindir}/minkidlc" |
|
Use |
I updated the bb file based on the comments, but the same issue occurred during compilation. error: no matching package named Here is the patch @@ -10,7 +10,7 @@ LIC_FILES_CHKSUM = "file://LICENSE.txt;md5=acff25b0ff46523fa016b260dbf64945"
SRC_URI = "git://github.com/quic/mink-idl-compiler.git;branch=main;protocol=https"
SRCREV = "1a52cd017ae21750c23e01851fdffd143f9c85d0"
-inherit cargo native
+cargo rust cargo-update-recipe-crates
+BBCLASSEXTEND += "native"
-DEPENDS += "rust-native cargo-native" |
jiaxli-QC
requested review from
harshaldev27,
lumag,
ricardosalveti and
vksharma-oss
lumag
left a comment
lumag
left a comment
There was a problem hiding this comment.
This wasn't tested:
ERROR: minkipc-0.0+git-r0 do_package: Didn't find service unit 'qteesupplicant.service', specified in SYSTEMD_SERVICE:minkipc.
dynamic-layers/openembedded-layer/recipes-devtools/mink-idl-compiler/mink-idl-compiler_0.2.3.bb
Show resolved
Hide resolved
dynamic-layers/openembedded-layer/recipes-devtools/minkidlc/minkidlc_0.2.1.bb
Outdated
Show resolved
Hide resolved
dynamic-layers/openembedded-layer/recipes-devtools/minkidlc/minkidlc_0.2.1.bb
Outdated
Show resolved
Hide resolved
| SRCREV = "18cbaabfb339c0751e239f2b2037bad52a0715b8" | ||
| PV = "0.0+git" | ||
|
|
||
| DEPENDS = "pkgconfig-native qcbor qcomtee minkidlc-native glib-2.0 systemd" |
There was a problem hiding this comment.
Add pkgconfig to inherits instead of having a raw dependency
There was a problem hiding this comment.
No, we should include the pkgconfig-native dependency; otherwise, a compilation issue may occur during configuration.
| CMake Error at qtee_supplicant/CMakeLists.txt:58 (install):
| install FILES given no DESTINATION!
|
|
| -- Configuring incomplete, errors occurred!
| WARNING: /local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/temp/run.do_configure.3923812:160 exit 1 from 'cmake
-G 'Ninja' -DCMAKE_MAKE_PROGRAM=ninja $oecmake_sitefile /local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/sources/
minkipc-0.0+git -DCMAKE_INSTALL_PREFIX:PATH=/usr -DCMAKE_INSTALL_BINDIR:PATH=bin -DCMAKE_INSTALL_SBINDIR:PATH=sbin -DCMAKE_INSTALL_LIBEXECDIR:PATH=lib
exec -DCMAKE_INSTALL_SYSCONFDIR:PATH=/etc -DCMAKE_INSTALL_SHAREDSTATEDIR:PATH=../com -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=/var -DCMAKE_INSTALL_LIBDIR:PA
TH=lib -DCMAKE_INSTALL_INCLUDEDIR:PATH=include -DCMAKE_INSTALL_DATAROOTDIR:PATH=share -DPYTHON_EXECUTABLE:PATH=/usr/bin/python3 -DPython_EXECUTABLE:PA
TH=/usr/bin/python3 -DPython3_EXECUTABLE:PATH=/usr/bin/python3 -DLIB_SUFFIX= -DCMAKE_INSTALL_SO_NO_EXE=0 -DCMAKE_TOOLCHAIN_FILE:FILEPATH=/local/mnt2/w
orkspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/toolchain.cmake -DCMAKE_NO_SYSTEM_FROM_IMPORTED=1 -DCMAKE_EXPORT_NO_PACKAGE_R
EGISTRY=ON -DFETCHCONTENT_FULLY_DISCONNECTED=ON -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON --log-level=VERBOSE --log-context -DBUILD_UNITTEST=ON -DCFG_EN
ABLE_SYSTEMD=ON -DCFG_USE_PKGCONFIG=ON -DMINKIDLC_BIN_DIR=/local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/recipe
-sysroot-native/usr/bin -Wno-dev'
| WARNING: Backtrace (BB generated script):
| #1: cmake_do_configure, /local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/temp/run.do_configure.3923812, l
ine 160
| #2: do_configure, /local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/temp/run.do_configure.3923812, line 13
9
| #3: main, /local/mnt2/workspace/K2L/upstream/build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/temp/run.do_configure.3923812, line 178
ERROR: Task (/local/mnt2/workspace/K2L/upstream/build/../meta-qcom/dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb:do_configure) failed with exit code '1'
NOTE: Tasks Summary: Attempted 1492 tasks of which 1491 didn't need to be rerun and 1 failed.
NOTE: Writing buildhistory
NOTE: Writing buildhistory took: 3 seconds
There was a problem hiding this comment.
Huh? I've just tested it with the following patch and it works (well... fails later).
diff --git a/dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb b/dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb
index f40b4c98f3f8..addd59fecdb5 100644
--- a/dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb
+++ b/dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb
@@ -11,13 +11,13 @@ SECTION = "devel"
LICENSE = "BSD-3-Clause"
LIC_FILES_CHKSUM = "file://LICENSE.txt;md5=2b1366ebba1ebd9ae25ad19626bbca93"
-inherit cmake systemd lib_package
+inherit cmake systemd lib_package pkgconfig
SRC_URI = "git://github.com/qualcomm/minkipc.git;branch=main;protocol=https"
SRCREV = "18cbaabfb339c0751e239f2b2037bad52a0715b8"
PV = "0.0+git"
-DEPENDS = "pkgconfig-native qcbor qcomtee minkidlc-native glib-2.0 systemd"
+DEPENDS = "qcbor qcomtee minkidlc-native glib-2.0 systemd"
EXTRA_OECMAKE = " \
-DBUILD_UNITTEST=ON \ERROR: minkipc-0.0+git-r0 do_package: Didn't find service unit 'qteesupplicant.service', specified in SYSTEMD_SERVICE:minkipc.
There was a problem hiding this comment.
BTW: I think you should also drop systemd from DEPENDS.
There was a problem hiding this comment.
Systemd is required for minkipc because the qtee_supplicant's CMakeLists.txt needs to access the systemd_system_unit_dir variable, as shown in https://github.com/qualcomm/minkipc/blob/main/qtee_supplicant/CMakeLists.txt#48.
There was a problem hiding this comment.
I was assuming systemd.bbclass would do DEPENDS += "systemd", but it doesn't. So adding it to DEPENDS here is valid.
Having said that, if you want to change DEPENDS after using inherit foo, you should use += to avoid overwriting the bbclass changes.
There was a problem hiding this comment.
@koenkooi but also think about non-systemd distros. We should not be adding the dependency here. Instead it will come through the distro features and a correct init manager being selected.
There was a problem hiding this comment.
Yeah, systemd is only used to find out the target directory, this recipe should have a PACKAGECONFIG for systemd, and include the dependency only when needed by the user (e.g. ${@bb.utils.filter('DISTRO_FEATURES', 'systemd', d)}).
CMakeLists.txt:option(CFG_USE_PKGCONFIG "Use pkg-config for discovering install target directory for systemd and udev files." OFF)
CMakeLists.txt:# Enable systemd to get variable from environment
CMakeLists.txt:option(CFG_ENABLE_SYSTEMD "Enable systemd service unit file generation." OFF)
qtee_supplicant/CMakeLists.txt: pkg_check_modules(SYSTEMD REQUIRED IMPORTED_TARGET systemd)
qtee_supplicant/CMakeLists.txt: if (SYSTEMD_FOUND AND CFG_ENABLE_SYSTEMD)
qtee_supplicant/CMakeLists.txt: pkg_get_variable(UNIT_DIR systemd systemd_system_unit_dir)
qtee_supplicant/CMakeLists.txt: set(SYSTEMD_UNIT_DIR "${UNIT_DIR}" CACHE PATH "Location of systemd unit files.")
qtee_supplicant/CMakeLists.txt:if (CFG_ENABLE_SYSTEMD)
qtee_supplicant/CMakeLists.txt: install(FILES "${CMAKE_CURRENT_BINARY_DIR}/qteesupplicant.service" DESTINATION "${SYSTEMD_UNIT_DIR}")
There was a problem hiding this comment.
No, that's a bad idea. Please don't add extra dependencies on systemd. Instead one should be able to pass systemd unit path through CMake conmmand line.
There was a problem hiding this comment.
Hi @lumag and @ricardosalveti can you confirm what is the final approach we should take here? Should we go with CMake cmd line approach then? Let us know so we can push an updated commit. 😃
dynamic-layers/openembedded-layer/recipes-devtools/minkidlc/minkidlc_0.2.1.bb
Outdated
Show resolved
Hide resolved
| -DMINKIDLC_BIN_DIR=${STAGING_BINDIR_NATIVE} \ | ||
| " | ||
|
|
||
| SYSTEMD_SERVICE:${PN} = "qteesupplicant.service" |
There was a problem hiding this comment.
You have been asked to split QTEE supplicant to a separate package. What happened?
There was a problem hiding this comment.
Hi @lumag , I responded above explaining the reason for qtee_supplicant being part of the minkipc package, quoting it here in-case it's missed.
Hello @ricardosalveti, the reason that qtee_supplicant is part of the minkipc repository is because it is an indispensable part of the Mink-IPC communication stack. It isn't just a client application which uses the Mink-IPC library to communicate with QTEE, it is a daemon hosting listener services which are invoked by QTEE to request access to REE (Rich Execution Environment such as Linux) hosted services such a File system and System time. Hosting it in a separate repo would decouple this important daemon from the Mink IPC stack, making many important and mandatory security services hosted in QTEE in-operational if the daemon isn't available on the system.
The OPTEE Linux stack also has a similar supplicant called tee-supplicant, which is also part of the same repo as the library which enables communication with OPTEE. Please refer: https://github.com/OP-TEE/optee_client/tree/master/tee-supplicant
There was a problem hiding this comment.
You replied why it is a part of the repo. We are asking to split it to a separate package, generated by the recipe.
dynamic-layers/openembedded-layer/recipes-devtools/minkidlc/minkidlc_0.2.1.bb
Outdated
Show resolved
Hide resolved
dynamic-layers/openembedded-layer/recipes-security/minkipc/minkipc_git.bb
Outdated
Show resolved
Hide resolved
| MINK ('Mink is Not a Kernel') is a capability-based security framework, \ | ||
| which is a synchronous message passing facility based on the Object-Capability model, \ | ||
| designed to facilitate secure communication between different domains. \ | ||
| qteesupplicant service is designed for invocation dispatch and handling callbacks. \ |
There was a problem hiding this comment.
Hello @ricardosalveti , we have a 1-1 mapping in-terms of functionality between all the binaries and libraries in optee_client and minkipc.
libteec -> libminkteec and/or libminkadaptor
tee-supplicant -> qtee_supplicant
The minkipc repository is also meant to be used by all the normal world client side exactly as you mentioned.
Also, I see the meta-freescale layer is installing all the contents of the optee_client repo, including the tee-supplicant binary as a single package: https://git.yoctoproject.org/meta-freescale/tree/recipes-security/optee-imx/optee-client_3.19.0.imx.bb?h=kirkstone
We want to ensure that any users familiar with OPTEE, and wishing to get a taste of QCOMTEE are immediately able to map familiar OPTEE functionality to QCOMTEE. And so, we are trying to follow them as closely as possible.
Let us know if you feel that a separate package is really the right way to go, in that case we can raise that change. 😄
jiaxli-QC
force-pushed
the
master
branch
2 times, most recently
from
c303704 to
53303af
Compare
dynamic-layers/openembedded-layer/recipes-devtools/minkidlc/minkidlc_0.2.1.bb
Outdated
Show resolved
Hide resolved
jiaxli-QC
requested review from
harshaldev27,
koenkooi,
lumag and
quaresmajose
jiaxli-QC
changed the title
ricardosalveti
left a comment
ricardosalveti
left a comment
There was a problem hiding this comment.
Rename minkidlc to mink-idl-compiler.
Provide qteesupplicant as a separated package in the minkipc recipe.
Both comments were already given multiple times, and still not yet addressed.
Hi @ricardosalveti , many thanks for you notification.
.../build/tmp/work/armv8-2a-qcom-linux/minkipc/0.0+git/packages-split$ tree minkipc minkipc-bin
minkipc
└── usr
└── lib
├── libfsservice.so.1 -> libfsservice.so.1.0.0
├── libfsservice.so.1.0.0
├── libgpfsservice.so.1 -> libgpfsservice.so.1.0.0
├── libgpfsservice.so.1.0.0
├── libminkadaptor.so.0 -> libminkadaptor.so.0.1.0
├── libminkadaptor.so.0.1.0
├── libminkteec.so.1 -> libminkteec.so.1.0.0
├── libminkteec.so.1.0.0
├── libtaautoload.so.1 -> libtaautoload.so.1.0.0
├── libtaautoload.so.1.0.0
├── libtimeservice.so.1 -> libtimeservice.so.1.0.0
├── libtimeservice.so.1.0.0
└── systemd
├── system
│ └── qteesupplicant.service
└── system-preset
└── 98-minkipc.preset
minkipc-bin
└── usr
└── bin
├── gp_test_client
├── qtee_supplicant
└── smcinvoke_client
2 directories, 3 filesThey've already been installed into different packages, how do we want to separate them? |
This is wrong. Moreover, I tried it here and it worked.
Clean the tmpdir completely and try again.
We have asked to put qteesupplicant in a separate package. No other debugging binaries in it. |
|
You also have the following files as part of the minkipc package: Add a minkipc-qteesupplicant package which includes both qtee_supplicant and qteesupplicant.service. |
The Mink-idl-compiler (minkidlc) is used by security components to convert .idl files into header files. Since minkipc depends on this tool and is part of dynamic-layer, add minkidlc to dynamic-layer. Signed-off-by: Jiaxing Li <[email protected]>
Minkipc is designed to facilitate secure communication between different domains. Add the minkipc component to dynamic-layer because it depends on the qcbor library from the meta-openembedded dynamic layer. Signed-off-by: Jiaxing Li <[email protected]>
Updated.
|
8 participants
Add the minkipc component to dynamic-layer because it depends on the qcbor library from the meta-openembedded dynamic layer.